FYST-792: ID Grocery Credit #4897

mrotondo · 2024-10-22T00:38:20Z

Link to pivotal/JIRA issue

https://codeforamerica.atlassian.net/browse/FYST-792

Is PM acceptance required? (delete one)

Yes - don't merge until JIRA issue is accepted!
Reminder: merge main into this branch and get green tests before merging to main

What was done?

Added the Idaho grocery credit - two new pages, calculator code, and XML + PDF output
We used the form & controller code from NameDob and AzSeniorDependents as examples

How to test?

We both tested manually and added automated unit tests for most scenarios
Risk Assessment
We added parameters to our "question-with-follow-up" javascript, so please also test some other places in the app where subsequent questions are revealed based on answers to make sure we didn't break them

Screenshots (for visual changes)

Co-authored-by: Mike Rotondo <[email protected]>

Co-authored-by: Hugo Melo <[email protected]>

github-actions · 2024-10-24T00:34:14Z

Heroku app: https://gyr-review-app-4897-3d3a6ab9ef27.herokuapp.com/
View logs: heroku logs --app gyr-review-app-4897 (optionally add --tail)

…rking on grocery credit review

…page styling

app/javascript/lib/honeycrisp.js

…r now

arinchoi03

Had a few questions! My head's spinning a bit tbh, might need to pair review tomorrow

app/views/state_file/questions/id_grocery_credit/edit.html.erb

db/migrate/20241025002538_add_donate_grocery_credit_to_id_intake.rb

config/locales/en.yml

app/lib/efile/id/id40_calculator.rb

app/lib/submission_builder/ty2024/states/id/documents/id40.rb

app/forms/state_file/id_grocery_credit_form.rb

app/views/hub/clients/_client_header.html.erb

app/javascript/lib/honeycrisp.js

…due to the existing override of honeycrisp's list--bulleted used elsewhere

… sure filers are synchronized in factory

Co-authored-by: Mike Rotondo <[email protected]>

…cting ineligible months

…s no

…ple'

… xml, and synchronize filers from json by default

…use that in Idaho to allow primary and spouse name and dob to be configured in tests

…ing primary/spouse info if it was provided to the factory

… overrides of default json contents with nil

arinchoi03

just a few more q's to clarify my understanding...this story got complex 😵

arinchoi03 · 2024-11-01T22:42:46Z

app/forms/state_file/id_grocery_credit_form.rb

+      updated_dependent_data = dependents_attributes
+
+      # set household member "has months" answers to no if household "has months" answer is no
+      if base_attrs[:household_has_grocery_credit_ineligible_months] == 'no'


this is just a non-related question that I also discussed just now with Drew, but why do we use enum instead of boolean for these types of fields?

Enums let us track yes/no/unfilled instead of just true/false, which I think is primarily useful when generating output for optional pages, though we don't check the unfilled value of this answer anywhere IIRC. Someone who's been on the team longer than me can give a better answer here :) I don't feel very strongly about it either way, mostly just following convention.

@DrewProebstel for more context

also @jenny-heath @embarnard @mpidcock @tahsinaislam if there was more reasoning for this choice (unfilled is similar to nil, no?)

FWIW I think that the decision to move away from using "unfilled" rather than nil as a default value in the app is well outside the scope of this PR, so discussion on that should probably happen elsewhere for posterity & visibility

arinchoi03 · 2024-11-04T04:29:50Z

app/forms/state_file/id_grocery_credit_form.rb

+      end
+
+      updated_dependent_data = updated_dependent_data.to_h do |k, v|
+        credit_count_key = v.key?(:id_months_ineligible_for_grocery_credit)


can we call this has_credit_count_key to be more clear

app/forms/state_file/id_grocery_credit_form.rb

app/models/df_json_wrapper.rb

arinchoi03 · 2024-11-04T04:45:21Z

app/models/df_json_wrapper.rb

+      selector_info => { type:, key: }
+      key_path = key.split
+      case type
+      when :date


should we also handle setting a boolean value ("true") or a money_amount value (& convert to the two decimal amount)? Just seeing that you handled the date and wondering why that one specifically

I believe but am not sure that dates are the only value whose string representation doesn't just already look the way we want, so they were the only one that needed extra formatting. But I was pretty deep into this story by the time I did this and was just trying to finish up, so maybe the others do need to be there! We don't use this for anything but strings & dates yet, in any case.

arinchoi03 · 2024-11-04T04:48:35Z

app/views/state_file/questions/id_grocery_credit_review/edit.html.erb

+    </div>
+
+    <div class="reveal">
+      <p><a href="#" class="reveal__link"><%= t('.donate_impact_heading') %></a></p>


change to <button class="reveal__button"> after merging in main

arinchoi03 · 2024-11-04T04:49:10Z

app/views/state_file/questions/id_grocery_credit/edit.html.erb

+    </div>
+
+    <div class="reveal">
+      <p><a href="#" class="reveal__link"><%= t('.why_are_you_asking_heading') %></a></p>


change to <button class="reveal__button"> after merging main

arinchoi03 · 2024-11-04T04:51:14Z

app/models/direct_file_json_data.rb

@@ -36,6 +36,10 @@ def initialize(json)
    @data = JSON.parse(json || "{}")
  end

+  def to_s
+    @data.to_s
+  end


why was this added?

Good catch. I originally added it because I incorrectly used it on state_file_id_intakes.rb:122 instead of to_json, but now that I look more closely, I introduced a bug when I switched over to calling to_json, as it's now returning a json string of the entire DirectFileJsonData object (i.e. it has {data: {...}}) instead of just its stored json object. So this needs to be changed to a to_json that exposes @data, e.g.

def to_json @data.to_json end

or

delegate :to_json, to: :data

anisharamnani · 2024-11-04T16:15:31Z

since @mrotondo is out today & we spent time working on this, i am going to give it a go addressing merge conflicts & handle some of the questions. 🤞

mrotondo · 2024-11-04T16:24:41Z

Thanks for hopping in @anisharamnani! Just popping my head up to answer a few questions.

# Conflicts: # app/javascript/lib/honeycrisp.js # app/lib/efile/id/id40_calculator.rb # app/lib/efile/line_data.yml # app/lib/navigation/state_file_id_question_navigation.rb # app/lib/pdf_filler/id40_pdf.rb # app/lib/submission_builder/ty2024/states/id/documents/id40.rb # app/models/state_file_base_intake.rb # app/models/state_file_id_intake.rb # config/locales/en.yml # config/locales/es.yml # db/schema.rb # spec/factories/state_file_id_intakes.rb # spec/features/state_file/complete_intake_spec.rb # spec/lib/efile/id/id40_calculator_spec.rb # spec/lib/submission_builder/ty2024/states/id/documents/id40_spec.rb # spec/models/efile_error_spec.rb # spec/models/state_file_id_intake_spec.rb

app/javascript/lib/honeycrisp.js

anisharamnani · 2024-11-04T21:29:29Z

hello @mrotondo! this is just a note for when you return, @mpidcock and i were able to make a merge in main and resolve the merge conflicts. 🎉

we've found that this issue you mentioned in slack is still occurring where if you select yes, then select no, the calculations still appear on the following page.

something may have changed with the merge conflicts to create this error? i am not sure, but we felt like we got as far as we could go with it.

Co-authored-by: Mike Rotondo <[email protected]>

…nabling/disabling followups, and ignore any dependents_attributes hashes that don't have ids in them

app/javascript/lib/honeycrisp.js

+                $(this).find('input').each(function (index, input) {
+                    if ($(this).attr('data-follow-up') != null) {
+                        if ($(this).is(':checked')) {
+                            $($(this).attr('data-follow-up')).find('input, select').attr('disabled', false);


To fix the problem, we need to ensure that the value extracted from the data-follow-up attribute is treated as a plain text string and not as HTML. This can be achieved by using a method that safely escapes any HTML meta-characters in the attribute value before using it in a jQuery selector.

The best way to fix this issue without changing existing functionality is to use the $.escapeSelector method provided by jQuery. This method escapes any characters that have special meaning in a CSS selector, thus preventing the possibility of XSS.

app/javascript/lib/honeycrisp.js

+                    if ($(this).attr('data-follow-up') != null) {
+                        if ($(this).is(':checked')) {
+                            $($(this).attr('data-follow-up')).find('input, select').attr('disabled', false);
+                            $($(this).attr('data-follow-up')).show();


To fix the problem, we need to ensure that the value of the data-follow-up attribute is safely used as a selector without the risk of executing arbitrary HTML or JavaScript. One way to achieve this is by using the $.find method instead of $, which interprets the value strictly as a CSS selector and not as HTML.

Replace the usage of $($(this).attr('data-follow-up')) with $(self).find($(this).attr('data-follow-up')) to ensure that the value is treated as a CSS selector within the context of the current element.

This change should be made on lines 109, 111, 132, and 133.

app/javascript/lib/honeycrisp.js

…erify that we fixed the nested followups/honeycrisp update bug

…om grocery credit month columns

wip

1f5edf5

mrotondo added the wip denotes a work in progress that isn't ready for formal review label Oct 22, 2024

mrotondo and others added 10 commits October 22, 2024 11:46

revert accidental change

2369a7f

remove unused household_members list

b9b7786

fix mis-indented divs

4f62674

Add toggle-able primary and dep months to id grocery

923d643

Co-authored-by: Mike Rotondo <[email protected]>

Add migration for grocery months id

039518f

Co-authored-by: Mike Rotondo <[email protected]>

adds primary and spouse to IdGroceryCreditForm

036e43d

Co-authored-by: Hugo Melo <[email protected]>

Merge branch 'main' into FYST-792-id-grocery-credit

925d2f6

Merge branch 'main' into FYST-792-id-grocery-credit

c7a22ff

Co-authored-by: Hugo Melo <[email protected]>

small cleanup to form

610fae2

Co-authored-by: Hugo Melo <[email protected]>

update intake and dependents and get page to render previous answers

f8b1361

mrotondo added 6 commits October 24, 2024 12:22

missed some annotate output after adding enums

e6c7050

added calculation for line 43, added some validations, and started wo…

9930e40

…rking on grocery credit review

refinements to calculator, xml & pdf output, more tests, more review …

8333df5

…page styling

add donate warning notice

c8f0de1

checkbox styling

b41bb7b

try modifying question-with-follow-up to handle our use cases

ad2a8dd

github-advanced-security bot found potential problems Oct 25, 2024

View reviewed changes

app/javascript/lib/honeycrisp.js Fixed Show fixed Hide fixed

app/javascript/lib/honeycrisp.js Fixed Show fixed Hide fixed

mrotondo added 6 commits October 25, 2024 17:31

page style and contents

e34b975

fix efile error spec

ff426ae

Merge branch 'main' into FYST-792-id-grocery-credit

fd42213

fix Idaho complete intake spec

bee9d71

make change to synchronize dependents only apply to specific trait fo…

f44ae01

…r now

remove todo we did

7ec2069

arinchoi03 reviewed Oct 28, 2024

View reviewed changes

mrotondo removed the wip denotes a work in progress that isn't ready for formal review label Oct 29, 2024

mrotondo added 2 commits October 29, 2024 16:49

move checkbox and ul styles into statefile stylesheet

3e52f03

add and use a household_count method on statefile base intake

aa30894

mrotondo and others added 15 commits October 30, 2024 12:27

simplify list styles, while still needing to use an additional class …

26f771f

…due to the existing override of honeycrisp's list--bulleted used elsewhere

a few additional expectations for thoroughness

2e24471

use new senior? helpers on intake instead of doing age math, and make…

950cb1c

… sure filers are synchronized in factory

use .and_call_original for default stub instead of returning nil

bebfdee

calculator changes mostly

57f82fa

Filter ineligible months for unchecked deps

d2576c3

Co-authored-by: Mike Rotondo <[email protected]>

check if household has grocery credit ineligible months before subtra…

707ee52

…cting ineligible months

set follow-up answers to no/nil in the form if the household answer i…

b9eefcf

…s no

fix Idaho complete intake test by creating a DF json for the 'old sam…

94f724e

…ple'

add a default json to the idaho intake factory, alongside the default…

5c2658f

… xml, and synchronize filers from json by default

allow direct file json to be written the same way we do the xml, and …

4c2f870

…use that in Idaho to allow primary and spouse name and dob to be configured in tests

add tests for json_writer

dcc7ba1

fix bug by changing to_s to to_json, and another one by only overwrit…

ab573cd

…ing primary/spouse info if it was provided to the factory

check pdf signature dates in the states' local time zones

5cda731

fix test of missing values (which should never be missing) by forcing…

29c68e1

… overrides of default json contents with nil

arinchoi03 reviewed Nov 4, 2024

View reviewed changes

github-advanced-security bot found potential problems Nov 4, 2024

View reviewed changes

app/javascript/lib/honeycrisp.js Fixed Show fixed Hide fixed

app/javascript/lib/honeycrisp.js Fixed Show fixed Hide fixed

Anisha Ramnani and others added 6 commits November 6, 2024 16:03

Remove invalid follow ups when there are no inelegible grocery credits

c8d4b83

Fix bug for invalid follow ups in the grocery credit form

15c6ce9

Co-authored-by: Mike Rotondo <[email protected]>

Merge honeycrisp updates with changes for nested follow ups

97d5804

Co-authored-by: Mike Rotondo <[email protected]>

WIP: expost intake bug and question when follow ups are deselected

892e5b6

Co-authored-by: Mike Rotondo <[email protected]>

simplify changes to honeycrisp

3fe82bc

disable followups by default, handle selects as well as inputs when e…

a534bea

…nabling/disabling followups, and ignore any dependents_attributes hashes that don't have ids in them

github-advanced-security bot found potential problems Nov 7, 2024

View reviewed changes

mrotondo added 2 commits November 6, 2024 17:44

restore & unbreak complete intake spec, and add new feature test to v…

26d9aac

…erify that we fixed the nested followups/honeycrisp update bug

fix bug in grocery credit form param handling and remove default 0 fr…

39c616f

…om grocery credit month columns

@@ -107,7 +107,7 @@
                                     if ($(this).is(':checked')) {
-                                        $($(this).attr('data-follow-up')).find('input, select').attr('disabled', false);
-                                        $($(this).attr('data-follow-up')).show();
+                                        $($.escapeSelector($(this).attr('data-follow-up'))).find('input, select').attr('disabled', false);
+                                        $($.escapeSelector($(this).attr('data-follow-up'))).show();
                                     } else {
-                                        $($(this).attr('data-follow-up')).find('input, select').attr('disabled', true);
-                                        $($(this).attr('data-follow-up')).hide();
+                                        $($.escapeSelector($(this).attr('data-follow-up'))).find('input, select').attr('disabled', true);
+                                        $($.escapeSelector($(this).attr('data-follow-up'))).hide();
                                     }

@@ -107,7 +107,7 @@
                                     if ($(this).is(':checked')) {
-                                        $($(this).attr('data-follow-up')).find('input, select').attr('disabled', false);
-                                        $($(this).attr('data-follow-up')).show();
+                                        $(self).find($(this).attr('data-follow-up')).find('input, select').attr('disabled', false);
+                                        $(self).find($(this).attr('data-follow-up')).show();
                                     } else {
-                                        $($(this).attr('data-follow-up')).find('input, select').attr('disabled', true);
-                                        $($(this).attr('data-follow-up')).hide();
+                                        $(self).find($(this).attr('data-follow-up')).find('input, select').attr('disabled', true);
+                                        $(self).find($(this).attr('data-follow-up')).hide();
                                     }
@@ -131,4 +131,4 @@
                                 if (/^[a-zA-Z0-9_\-#\.]+$/.test(followUpSelector)) {
-                                    $(followUpSelector).show();
-                                    $(followUpSelector).find('input, select').attr('disabled', false);
+                                    $container.find(followUpSelector).show();
+                                    $container.find(followUpSelector).find('input, select').attr('disabled', false);
                                 }

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FYST-792: ID Grocery Credit #4897

FYST-792: ID Grocery Credit #4897

mrotondo commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024

arinchoi03 left a comment •

edited

Loading

arinchoi03 left a comment •

edited

Loading

arinchoi03 Nov 1, 2024

mrotondo Nov 4, 2024

arinchoi03 Nov 6, 2024

mrotondo Nov 6, 2024

arinchoi03 Nov 4, 2024

arinchoi03 Nov 4, 2024 •

edited

Loading

mrotondo Nov 4, 2024 •

edited

Loading

arinchoi03 Nov 4, 2024

arinchoi03 Nov 4, 2024

arinchoi03 Nov 4, 2024

mrotondo Nov 4, 2024

anisharamnani commented Nov 4, 2024

mrotondo commented Nov 4, 2024

anisharamnani commented Nov 4, 2024

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

FYST-792: ID Grocery Credit #4897

Are you sure you want to change the base?

FYST-792: ID Grocery Credit #4897

Conversation

mrotondo commented Oct 22, 2024 • edited Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

What was done?

How to test?

Screenshots (for visual changes)

github-actions bot commented Oct 24, 2024

arinchoi03 left a comment • edited Loading

Choose a reason for hiding this comment

arinchoi03 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arinchoi03 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

mrotondo Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anisharamnani commented Nov 4, 2024

mrotondo commented Nov 4, 2024

anisharamnani commented Nov 4, 2024

mrotondo commented Oct 22, 2024 •

edited

Loading

arinchoi03 left a comment •

edited

Loading

arinchoi03 left a comment •

edited

Loading

arinchoi03 Nov 4, 2024 •

edited

Loading

mrotondo Nov 4, 2024 •

edited

Loading